rustPlatform.fetchCargoTarball: support pname+version #332975

eclairevoyant · 2024-08-07T11:02:43Z

Description of changes

rustPlatform.fetchCargoTarball: nixfmt
rustPlatform.fetchCargoTarball: support pname, version
- tracking issue: Change all packages from "name" to "pname"+"version", remove all uses of meta.version and meta.tag #103997
doc/rust: prefer pname+version over name in fetchCargoTarball
mousai: change name -> pname+version in cargoDeps
- simply done as a proof-of-concept, should result in 0 rebuilds
treewide: remove existing usages of pname+version in fetchCargoTarball, that incorrectly assumed it was supported (prevents hash mismatches)

Ths should result in exactly one rebuild (the manual).

Things done

Add a 👍 reaction to pull requests you find important.

pkgs/build-support/rust/fetch-cargo-tarball/default.nix

eclairevoyant · 2024-08-10T01:47:03Z

Funnily enough, it seem like there's several packages that already assumed that fetchCargoTarball could accept pname+version, resulting in hash mismatches after this PR. Therefore, to preserve this as a zero-rebuild PR, I'd have to not-pass those attrs in those cases.

Someone actually almost noticed this in python312Packages.datafusion but didn't seem to understand why.

eclairevoyant · 2024-08-10T02:10:25Z

Okay, this now has one rebuild (the nixpkgs manual) which makes sense and I can live with.

Result of nixpkgs-review run on x86_64-linux 1

1 package built:

nixpkgs-manual

eclairevoyant · 2024-08-10T02:51:17Z

Thanks for agreeing borgo

emilazy

LGTM in principle, but I think I’d prefer updating the hashes in e878dfe145e5f71d5a5817f5244ea8002865ccc5 to dropping what is now meant to be good practice.

eclairevoyant · 2024-08-10T14:00:12Z

It's certainly possible to follow up with a treewide in another PR, but I didn't want to balloon the scope here too much, or make it harder to review or merge without conflicts. I'm sure there are many packages that don't set name at all, that we'd want to fix anyway.

nixos-discourse · 2024-09-01T00:42:15Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4496

pkgs/applications/audio/gnome-podcasts/default.nix

philiptaron

I think this looks good to merge. Barring any objections, I intend to do so in a day.

pkgs/build-support/rust/fetch-cargo-tarball/default.nix

philiptaron · 2024-09-05T15:43:01Z

pkgs/build-support/rust/fetch-cargo-tarball/default.nix

+
+    inherit (hash_) outputHashAlgo outputHash;
+
+    impureEnvVars = lib.fetchers.proxyImpureEnvVars ++ [ "NIX_CRATES_INDEX" ];


Future: document this environment variable somewhere. Maybe in the Rust section of the nixpkgs-manual?

philiptaron · 2024-09-05T15:44:34Z

pkgs/build-support/rust/fetch-cargo-tarball/default.nix

+      cat >$CARGO_HOME/config.toml <<EOF
+      [source.crates-io]
+      replace-with = 'mirror'
+      [source.mirror]
+      registry = "$NIX_CRATES_INDEX"
+      EOF


Needs one more level of indent.

The indentation error is present in the base branch. ~~, but sure, I can fix it~~

Actually if I indent this then the contents of the toml file will be indented.
The only line that's indentable is L93 but that might look even more confusing 🫤

Wow, load-bearing odd indentation. Thanks Bash.

philiptaron · 2024-09-05T15:45:54Z

pkgs/build-support/rust/fetch-cargo-tarball/default.nix

+          echo
+          echo "ERROR: The Cargo.lock file doesn't exist"
+          echo
+          echo "Cargo.lock is needed to make sure that cargoHash/cargoSha256 doesn't change"
+          echo "when the registry is updated."
+          echo
+
+          exit 1


In Nix files, I do like my shell to have two-space indent. .editorconfig says four, but @emilazy and I might connive to effect a two-space-coup on shell.

If you hide whitespace (https://github.com/NixOS/nixpkgs/pull/332975/files?diff=unified&w=1) you'll see I didn't substantively touch these lines at all. These lines are indented in the diff, but only by the same amount that the surroundings are indented (and was done by nixfmt, not manually).

Oh yes, this is just me noticing formatting due to, well, the formatting being impacted with nixfmt.

philiptaron · 2024-09-05T15:50:38Z

pkgs/by-name/ch/chance/package.nix

@@ -26,7 +26,7 @@ stdenv.mkDerivation (finalAttrs: {
  };

  cargoDeps = rustPlatform.fetchCargoTarball {
-    inherit (finalAttrs) pname version src;
+    inherit (finalAttrs) src;


According to the PR headmatter, this is removed in order to not cause hash mismatches.

philiptaron · 2024-09-05T16:50:45Z

pkgs/build-support/rust/fetch-cargo-tarball/default.nix

+      cat >$CARGO_HOME/config.toml <<EOF
+      [source.crates-io]
+      replace-with = 'mirror'
+      [source.mirror]
+      registry = "$NIX_CRATES_INDEX"
+      EOF


Wow, load-bearing odd indentation. Thanks Bash.

This is done because the existing fetchCargoTarball does not support pname or version, resulting in a vendor tarball with the name cargo-deps-vendor.tar.gz. Since adding pname+version support would change the name of the derivation, and therefore its hash, we remove existing usages to avoid treewide breakage.

eclairevoyant requested review from zowoq, winterqt and figsoda as code owners August 7, 2024 11:02

github-actions bot added 8.has: documentation 6.topic: rust labels Aug 7, 2024

eclairevoyant force-pushed the fetchcargotarball-pname branch from 32e4fb2 to 28dc9f4 Compare August 7, 2024 11:03

emilazy reviewed Aug 7, 2024

View reviewed changes

pkgs/build-support/rust/fetch-cargo-tarball/default.nix Show resolved Hide resolved

eclairevoyant marked this pull request as draft August 7, 2024 11:35

eclairevoyant changed the title ~~rustPlatform.fetchCargoTarball: support pname+version~~ [WIP] rustPlatform.fetchCargoTarball: support pname+version Aug 7, 2024

eclairevoyant force-pushed the fetchcargotarball-pname branch from 28dc9f4 to cb33492 Compare August 7, 2024 12:37

ofborg bot added 8.has: clean-up 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Aug 7, 2024

eclairevoyant force-pushed the fetchcargotarball-pname branch from cb33492 to b37eefd Compare August 10, 2024 01:21

github-actions bot added the 6.topic: python label Aug 10, 2024

eclairevoyant force-pushed the fetchcargotarball-pname branch from 5870d93 to e878dfe Compare August 10, 2024 02:01

eclairevoyant marked this pull request as ready for review August 10, 2024 02:10

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Aug 10, 2024

eclairevoyant changed the title ~~[WIP] rustPlatform.fetchCargoTarball: support pname+version~~ rustPlatform.fetchCargoTarball: support pname+version Aug 10, 2024

eclairevoyant requested review from emilazy and Aleksanaa August 10, 2024 02:51

emilazy approved these changes Aug 10, 2024

View reviewed changes

eclairevoyant added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Aug 10, 2024

eclairevoyant requested a review from alyssais August 19, 2024 01:19

SuperSandro2000 reviewed Sep 3, 2024

View reviewed changes

pkgs/applications/audio/gnome-podcasts/default.nix Outdated Show resolved Hide resolved

philiptaron approved these changes Sep 5, 2024

View reviewed changes

eclairevoyant force-pushed the fetchcargotarball-pname branch from e878dfe to 5ff3fef Compare September 5, 2024 16:49

philiptaron approved these changes Sep 5, 2024

View reviewed changes

eclairevoyant force-pushed the fetchcargotarball-pname branch from 5ff3fef to 0278111 Compare September 5, 2024 16:54

eclairevoyant added 5 commits September 7, 2024 01:09

rustPlatform.fetchCargoTarball: nixfmt

1f0f59d

rustPlatform.fetchCargoTarball: support pname, version

3207bc2

doc/rust: prefer pname+version over name in fetchCargoTarball

5c38beb

mousai: change name -> pname+version in cargoDeps

5588371

eclairevoyant force-pushed the fetchcargotarball-pname branch from 0278111 to e273bc8 Compare September 7, 2024 05:10

philiptaron approved these changes Sep 7, 2024

View reviewed changes

philiptaron merged commit 29cca09 into NixOS:master Sep 7, 2024
24 of 27 checks passed

eclairevoyant deleted the fetchcargotarball-pname branch September 7, 2024 14:34

TomaSajt mentioned this pull request Sep 9, 2024

keypunch: 1.0 -> 3.1 #340766

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustPlatform.fetchCargoTarball: support pname+version #332975

rustPlatform.fetchCargoTarball: support pname+version #332975

eclairevoyant commented Aug 7, 2024 •

edited

Loading

eclairevoyant commented Aug 10, 2024 •

edited

Loading

eclairevoyant commented Aug 10, 2024

eclairevoyant commented Aug 10, 2024

emilazy left a comment

eclairevoyant commented Aug 10, 2024 •

edited

Loading

nixos-discourse commented Sep 1, 2024

philiptaron left a comment

philiptaron Sep 5, 2024

philiptaron Sep 5, 2024

eclairevoyant Sep 5, 2024 •

edited

Loading

philiptaron Sep 5, 2024

philiptaron Sep 5, 2024

eclairevoyant Sep 5, 2024

philiptaron Sep 5, 2024

philiptaron Sep 5, 2024

philiptaron Sep 5, 2024


		inherit (hash_) outputHashAlgo outputHash;

		impureEnvVars = lib.fetchers.proxyImpureEnvVars ++ [ "NIX_CRATES_INDEX" ];

rustPlatform.fetchCargoTarball: support pname+version #332975

rustPlatform.fetchCargoTarball: support pname+version #332975

Conversation

eclairevoyant commented Aug 7, 2024 • edited Loading

Description of changes

Things done

eclairevoyant commented Aug 10, 2024 • edited Loading

eclairevoyant commented Aug 10, 2024

eclairevoyant commented Aug 10, 2024

emilazy left a comment

Choose a reason for hiding this comment

eclairevoyant commented Aug 10, 2024 • edited Loading

nixos-discourse commented Sep 1, 2024

philiptaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclairevoyant Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclairevoyant commented Aug 7, 2024 •

edited

Loading

eclairevoyant commented Aug 10, 2024 •

edited

Loading

eclairevoyant commented Aug 10, 2024 •

edited

Loading

eclairevoyant Sep 5, 2024 •

edited

Loading